Properly implement WaitForData for ReadConsoleInput #18228

lhecker · 2024-11-21T00:42:24Z

There were two bugs:

Ever since the conhost v1 -> v2 rewrite the readDataDirect.cpp
implementation incorrectly passed false as the wait flag.
The unintentional mistake is obvious in hindsight as the
check for CONSOLE_STATUS_WAIT makes no sense in this case.
The ConPTY integration into InputBuffer was done incorrectly,
as it would unconditionally wake up the readers/waiters without
checking if the buffer is now actually non-empty.

Validation Steps Performed

Test code:

#include <Windows.h>
#include <stdio.h>

int main() {
    HANDLE in = GetStdHandle(STD_INPUT_HANDLE);
    INPUT_RECORD buf[128];
    DWORD read;

    SetConsoleMode(
        in,
        ENABLE_PROCESSED_INPUT | ENABLE_VIRTUAL_TERMINAL_INPUT
    );

    for (int i = 0; ReadConsoleInputW(in, buf, 128, &read); ++i) {
        printf("%d read=%lu\n", i, read);
    }

    return 0;
}

Run it under Windows Terminal and type any input. >50% of all
inputs will result in read=0. This is fixed after this PR.

lhecker · 2024-11-21T00:44:50Z

src/host/inputBuffer.cpp

@@ -492,8 +492,7 @@ size_t InputBuffer::Prepend(const std::span<const INPUT_RECORD>& inEvents)
            return STATUS_SUCCESS;
        }

-        _vtInputShouldSuppress = true;
-        auto resetVtInputSuppress = wil::scope_exit([&]() { _vtInputShouldSuppress = false; });
+        const auto wakeup = _wakeupReadersOnExit();


Much cleaner and safer. Also, it's more correct. We would previously call WakeUpReadersWaitingForData unconditionally but the storage could still be empty. This error has probably been in conhost since v1.

I can't remember 4 years ago why _vtInputShouldSuppress and input wake muffling was added...

It was due to the "circular" control flow, as _termInput was set up with the callback (boo!) _HandleTerminalInputCallback. Additionally, the Prepend was implemented with a series of 2 "Append"s. The first one would process the input and call into _termInput which would then invoke _HandleTerminalInputCallback. This meant that the event was flagged and the waiters woken up before the code had finished to append the 2nd half. To fix this, the flag was added to make the callback stop waking the waiters up.

This PR solves the "knot" by moving all of the waking logic out of the internal append-function and into the outermost, public functions. Now they have perfect control over when to wake up something and when to not do it.

lhecker · 2024-11-21T00:45:45Z

src/host/inputBuffer.cpp

-// - inEvents - Series of input records to insert into the buffer
-// Return Value:
-// - <none>
-void InputBuffer::_HandleTerminalInputCallback(const TerminalInput::StringType& text)


The second fix. It's also redundant with _writeString, especially with _vtInputShouldSuppress kicked out.

lhecker · 2024-11-21T00:45:56Z

src/host/readDataDirect.cpp

@@ -107,7 +107,7 @@ try
    *pReplyStatus = _pInputBuffer->Read(_outEvents,
                                        amountToRead,
                                        false,
-                                        false,
+                                        true,


The first fix.

lhecker · 2024-11-21T00:46:44Z

src/server/ApiDispatchers.cpp

-        if (fIsWaitAllowed)
-        {
-            hr = ConsoleWaitQueue::s_CreateWait(m, waiter.release());
-            if (SUCCEEDED(hr))
-            {
-                *pbReplyPending = TRUE;
-                hr = CONSOLE_STATUS_WAIT;
-            }
-        }


Makes no sense IMO to check this here if we can just pass down fIsWaitAllowed and avoid a waiter from being created in the first place.

carlos-zamora · 2024-11-25T21:53:22Z

OpenConsole.sln

-        {7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.ActiveCfg = Debug|Win32
-        {7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.ActiveCfg = Release|ARM64
-        {7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.ActiveCfg = Release|x64
-        {7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.ActiveCfg = Release|Win32
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.ActiveCfg = AuditMode|x64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.Build.0 = AuditMode|x64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.ActiveCfg = AuditMode|ARM64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.Build.0 = AuditMode|ARM64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.ActiveCfg = AuditMode|x64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.Build.0 = AuditMode|x64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.ActiveCfg = AuditMode|Win32
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.Build.0 = AuditMode|Win32


(Double checking) intentional?

Huh, I was certain I reverted this...

carlos-zamora · 2024-11-25T21:54:22Z

src/host/directio.cpp

@@ -56,6 +56,7 @@ using Microsoft::Console::Interactivity::ServiceLocator;
                                                       INPUT_READ_HANDLE_DATA& readHandleState,
                                                       const bool IsUnicode,
                                                       const bool IsPeek,
+                                                       const bool IsWaitAllowed,


nit: update the function comment above

carlos-zamora · 2024-11-25T22:02:43Z

src/host/inputBuffer.cpp

@@ -702,13 +674,11 @@ static bool IsPauseKey(const KEY_EVENT_RECORD& event)
 // Note:
 // - The console lock must be held when calling this routine.
 // - will throw on failure
-void InputBuffer::_WriteBuffer(const std::span<const INPUT_RECORD>& inEvents, _Out_ size_t& eventsWritten, _Out_ bool& setWaitEvent)


nit: setWaitEvent is still documented above. It should be removed.

…d-console-input

Properly implement WaitForData for ReadConsoleInput

2388624

lhecker force-pushed the dev/lhecker/15859-read-console-input branch from dab80c8 to 2388624 Compare November 21, 2024 00:43

lhecker commented Nov 21, 2024

View reviewed changes

carlos-zamora approved these changes Nov 25, 2024

View reviewed changes

lhecker added 3 commits December 3, 2024 17:18

Merge remote-tracking branch 'origin/main' into dev/lhecker/15859-rea…

4607d24

…d-console-input

Address feedback

402b21e

Fix AuditMode & tests

828fae1

DHowett approved these changes Dec 3, 2024

View reviewed changes

lhecker merged commit 5c55144 into main Dec 3, 2024
20 checks passed

lhecker deleted the dev/lhecker/15859-read-console-input branch December 3, 2024 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly implement WaitForData for ReadConsoleInput #18228

Properly implement WaitForData for ReadConsoleInput #18228

lhecker commented Nov 21, 2024

lhecker Nov 21, 2024

DHowett Dec 3, 2024

lhecker Dec 3, 2024 •

edited

Loading

lhecker Nov 21, 2024

lhecker Nov 21, 2024

lhecker Nov 21, 2024

carlos-zamora Nov 25, 2024

lhecker Nov 25, 2024

carlos-zamora Nov 25, 2024

carlos-zamora Nov 25, 2024

Properly implement WaitForData for ReadConsoleInput #18228

Properly implement WaitForData for ReadConsoleInput #18228

Conversation

lhecker commented Nov 21, 2024

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Dec 3, 2024 •

edited

Loading